Add UseVolatileReadWriteAnalyzer #7043

CollinAlpert · 2023-11-19T11:24:30Z

This PR adds an analyzer which flags invocations to Thread.VolatileRead and Thread.VolatileWrite and a fixer which rewrites the calls to Volatile.Read and Volatile.Write respectively.

Fixes dotnet/runtime#27997

stephentoub · 2023-11-19T17:32:19Z

This PR adds an analyzer

Thanks, though I thought the plan was to mark them as obsolete and then have a fixer associated with that diagnostic id? Or did that plan change?

CollinAlpert · 2023-11-19T17:46:22Z

That was my understanding as well, however I couldn't figure out how to add tests without an analyzer emitting the diagnostic ID and I also thought it would benefit developers immediately instead of having to wait for the obsoletion of the APIs.

But I am happy to delete the analyzer if someone can let me know how to configure the test.

mavasani · 2023-11-29T08:38:57Z

But I am happy to delete the analyzer if someone can let me know how to configure the test.

@CollinAlpert You can move the analyzer over to the test project and use it only for the tests. Add a comment + open a tracking issue to remove this analyzer in the future.

Please do make sure that the code fixer and this test-only analyzer use the Diagnostic ID that will be used in the Obsolete attribute on the APIs - I am not sure if a CAxxxx diagnostic ID is appropriate to indicate obsoletion, maybe they should use a special prefix, I see we have https://learn.microsoft.com/en-us/dotnet/fundamentals/syslib-diagnostics/obsoletions-overview#reference. I think you should first confirm the diagnostic ID in the issue and ensure it doesn't get used by another feature.

# Conflicts: # src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx

codecov · 2023-12-05T18:25:07Z

Codecov Report

Attention: Patch coverage is 99.00332% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 96.47%. Comparing base (7b1f39d) to head (8284db4).
Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #7043    +/-   ##
========================================
  Coverage   96.46%   96.47%            
========================================
  Files        1432     1436     +4     
  Lines      342185   342787   +602     
  Branches    11280    11289     +9     
========================================
+ Hits       330104   330708   +604     
+ Misses       9230     9225     -5     
- Partials     2851     2854     +3

CollinAlpert · 2024-02-07T19:39:37Z

@mavasani What's missing here? I received no response in the issue, so I am assuming the diagnostic ID is okay. Is there anything else I need to do before this can be merged?

# Conflicts: # src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx

buyaa-n · 2024-03-02T00:57:58Z

@mavasani @stephentoub if the fixer should be fired for Obsoletion diagnostic ID is this repo is the correct place for the fixer? As far as I know most SYSLIB**** analyzer/fixers implemented in runtime, but I think the obsoletion analyzer is in roslyn repo. Adding the fixer here might still work though.

What's missing here? I received no response in the issue, so I am assuming the diagnostic ID is okay. Is there anything else I need to do before this can be merged?

Sorry for delay @CollinAlpert, we can's assume the diagnostic ID used here is okay, anybody might use that ID for obsoleting other API (the rule is just grab next available ID I think). You would need assign the ID for this obsoletion and mark the related APIs with that check this https://github.com/dotnet/runtime/pull/84812/files for example. But we might not want to mark the APIs until the fixer is implemented, not sure, what you think @stephentoub?

buyaa-n · 2024-03-02T01:13:20Z

Actually with dotnet/roslyn-sdk#1117 being fixed seems now we can add the fixer here. Without having an anlyzer for testing only.

CollinAlpert · 2024-03-02T10:23:32Z

So I would need to create a PR for dotnet/runtime first, adding the [Obsolete] attributes?

mpidash · 2024-03-02T16:55:49Z

Actually with dotnet/roslyn-sdk#1117 being fixed seems now we can add the fixer here. Without having an anlyzer for testing only.

@CollinAlpert I pushed my WIP implementation of a fixer that uses no additional analyzer (with the fix from Roslyn SDK), see here: mpidash@39276c6#diff-b057b03b5cca1496dc3d962dd2593bfefe06119259299f588966b2c344a78ddeR20-R73
I also only used the IOperation API instead of checking the syntax in language specific fixers. Not sure which is preferred though.

Feel free to copy anything you need.

mpidash · 2024-03-02T17:14:59Z

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Usage/UseVolatileReadWrite.Fixer.cs

+                var codeAction = CodeAction.Create(
+                    MicrosoftNetCoreAnalyzersResources.UseVolatileReadTitle,
+                    _ => Task.FromResult(context.Document.WithSyntaxRoot(root.ReplaceNode(readAccess, CreateVolatileMemberAccess(context.Document, VolatileReadMethodName)))),
+                    MicrosoftNetCoreAnalyzersResources.UseVolatileReadTitle


Nit (see #5431):

Suggested change

MicrosoftNetCoreAnalyzersResources.UseVolatileReadTitle

nameof(MicrosoftNetCoreAnalyzersResources.UseVolatileReadTitle)

mpidash · 2024-03-02T17:15:48Z

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Usage/UseVolatileReadWrite.Fixer.cs

+                var codeAction = CodeAction.Create(
+                    MicrosoftNetCoreAnalyzersResources.UseVolatileWriteTitle,
+                    _ => Task.FromResult(context.Document.WithSyntaxRoot(root.ReplaceNode(writeAccess, CreateVolatileMemberAccess(context.Document, VolatileWriteMethodName)))),
+                    MicrosoftNetCoreAnalyzersResources.UseVolatileWriteTitle


Nit (see #5431):

Suggested change

MicrosoftNetCoreAnalyzersResources.UseVolatileWriteTitle

nameof(MicrosoftNetCoreAnalyzersResources.UseVolatileWriteTitle)

mpidash · 2024-03-02T17:25:56Z

src/NetAnalyzers/CSharp/Microsoft.NetCore.Analyzers/Usage/CSharpUseVolatileReadWriteFixer.cs

+    [ExportCodeFixProvider(LanguageNames.CSharp), Shared]
+    internal sealed class CSharpUseVolatileReadWriteFixer : UseVolatileReadWriteFixer
+    {
+        protected override bool TryGetThreadVolatileReadWriteMemberAccess(SyntaxNode invocation, string methodName, [NotNullWhen(true)] out SyntaxNode? memberAccess)


Suggested change

protected override bool TryGetThreadVolatileReadWriteMemberAccess(SyntaxNode invocation, string methodName, [NotNullWhen(true)] out SyntaxNode? memberAccess)

protected sealed override bool TryGetThreadVolatileReadWriteMemberAccess(SyntaxNode invocation, string methodName, [NotNullWhen(true)] out SyntaxNode? memberAccess)

mpidash · 2024-03-02T17:29:22Z

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Usage/UseVolatileReadWrite.Fixer.cs

+            {
+                var codeAction = CodeAction.Create(
+                    MicrosoftNetCoreAnalyzersResources.UseVolatileReadTitle,
+                    _ => Task.FromResult(context.Document.WithSyntaxRoot(root.ReplaceNode(readAccess, CreateVolatileMemberAccess(context.Document, VolatileReadMethodName)))),


I think this breaks with named arguments, as Volatile.Read and Thread.VolatileRead use different parameter names (location and address).

mpidash · 2024-03-02T17:30:03Z

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Usage/UseVolatileReadWrite.Fixer.cs

+            );
+        }
+
+        public override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer;


Suggested change

public override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer;

public sealed override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer;

mpidash · 2024-03-02T17:30:34Z

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Usage/UseVolatileReadWrite.Fixer.cs

+
+        protected abstract bool TryGetThreadVolatileReadWriteMemberAccess(SyntaxNode invocation, string methodName, [NotNullWhen(true)] out SyntaxNode? memberAccess);
+
+        public override ImmutableArray<string> FixableDiagnosticIds { get; } = ImmutableArray.Create("SYSLIB0054");


Suggested change

public override ImmutableArray<string> FixableDiagnosticIds { get; } = ImmutableArray.Create("SYSLIB0054");

public sealed override ImmutableArray<string> FixableDiagnosticIds { get; } = ImmutableArray.Create("SYSLIB0054");

mpidash · 2024-03-02T17:36:43Z

src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Usage/UseVolatileReadWriteTests.cs

+
+namespace Microsoft.NetCore.Analyzers.Usage.UnitTests
+{
+    public sealed class UseVolatileReadWriteTests


Tests checking if trivia and named arguments are preserved are missing.

buyaa-n · 2024-03-04T03:11:59Z

So I would need to create a PR for dotnet/runtime first, adding the [Obsolete] attributes?

In general yes, that includes adding the attributes to the APIs with the desired ID and message, find all usages and fix them, though having a fixer could help with fixing if there are many usages. Though, I would say it can be optional, add the attributes if you would like to, or continue implementing the fixer with SYSLIB0054 ID. In case it is used for another obsoletion we could change it later, no big deal.

buyaa-n · 2024-03-04T03:19:11Z

I pushed my WIP implementation of a fixer that uses no additional analyzer (with the fix from Roslyn SDK)

Thank you very much @mpidash, implementing a fixer that uses no additional analyzer is the what we need here

I also only used the IOperation API instead of checking the syntax in language specific fixers. Not sure which is preferred though.

Definitely IOperation APIs should be used whenever possible, only parts that is not supported with IOperation should use language specific implementations

# Conflicts: # src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx

CollinAlpert · 2024-03-06T21:25:47Z

Definitely IOperation APIs should be used whenever possible

@buyaa-n But doesn't IOperation require a semantic model and thus have a large performance overhead?

CollinAlpert · 2024-03-06T22:09:22Z

@mpidash Thanks a lot, your WIP was extremely helpful! :)

buyaa-n · 2024-03-06T22:31:24Z

@buyaa-n But doesn't IOperation require a semantic model and thus have a large performance overhead?

I don't really know the performance impact, but I would not expect perf overhead, @mavasani @sharwell could reply to that. I know that IOperation APIs are newer and added to avoid language specific implementations. Writing one code and using for all languages is better at least for maintenance, code reuse perspective.

mavasani · 2024-03-07T04:41:35Z

@buyaa-n But doesn't IOperation require a semantic model and thus have a large performance overhead?

I don't really know the performance impact, but I would not expect perf overhead, @mavasani @sharwell could reply to that. I know that IOperation APIs are newer and added to avoid language specific implementations. Writing one code and using for all languages is better at least for maintenance, code reuse perspective.

There should be no negative performance impact on using IOperation APIs in analyzers and fixers. We already have a whole ecosystem of IOperation based analyzers and fixers, and they all share the same IOperation trees.

CollinAlpert · 2024-03-07T17:29:41Z

I switched to using IOperation APIs, however I wasn't able to make named arguments work without language-specific fixers.

I also had to disable the test VB_UseVolatileWrite_WithReversedArguments, since Visual Basic's arguments are not passed to the fixer in the order they are supplied to the method. I figure it is fine for the user to have to switch the order of the arguments themselves. Besides, the fixed code will still compile.

buyaa-n · 2024-03-07T23:09:44Z

I switched to using IOperation APIs, however I wasn't able to make named arguments work without language-specific fixers.

Thanks, I believe you can use this extension method for that:

roslyn-analyzers/src/Utilities/Compiler/Extensions/IOperationExtensions.cs

Lines 884 to 888 in 7b1f39d

    
           /// Useful when named arguments used for a method call and you need them in the original parameter order. 
        
           /// </summary> 
        
           /// <param name="arguments">Arguments of the method</param> 
        
           /// <returns>Returns the arguments in parameter order</returns> 
        
           public static ImmutableArray<IArgumentOperation> GetArgumentsInParameterOrder(

i.e. always populate the arguments in order

CollinAlpert · 2024-03-07T23:37:53Z

@buyaa-n You mean when someone calls Thread.VolatileRead(value: value, address: ref address) the call should be rewritten to Volatile.Read(location: ref address, value: value) or even Volatile.Read(ref address, value)?

buyaa-n · 2024-03-07T23:49:10Z

Yes, replacing with Volatile.Read(location: ref address, value: value) is completely acceptable, if that is not doable, I am OK with Volatile.Read(ref address, value) too.

CollinAlpert · 2024-03-11T19:59:54Z

@buyaa-n Done. Unfortunately I still need the language-specific fixers, since Thread.VolatileWrite's and Volatile.Write's argument names are different (address vs. location). Same with Thread.VolatileWrite and Volatile.Read.

buyaa-n · 2024-03-11T23:48:12Z

@buyaa-n Done. Unfortunately I still need the language-specific fixers, since Thread.VolatileWrite's and Volatile.Write's argument names are different (address vs. location). Same with Thread.VolatileWrite and Volatile.Read.

OK, then always populate the arguments in order was not needed, I reverted it and enabled the VB test for reverse order to validate the fix in order

buyaa-n

LGTM, thank you @CollinAlpert

Add UseVolatileReadWriteAnalyzer

955b243

CollinAlpert requested a review from a team as a code owner November 19, 2023 11:24

github-actions bot added the Community label Nov 19, 2023

CollinAlpert added 5 commits November 30, 2023 11:30

Merge branch 'master' into issue-27997

2fa40e3

Move analyzer to test project.

fea9ab0

Update RulesMissingDocumentation.md

df65c56

Merge branch 'master' into issue-27997

877922f

# Conflicts: # src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx

Add copyright comment

6fe41cb

Merge branch 'master' into issue-27997

a00b684

Merge branch 'master' into issue-27997

4fe1cad

# Conflicts: # src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx

buyaa-n mentioned this pull request Mar 2, 2024

Obsoletion diagnostic IDs (SYSLIB0xxx) get filtered out in CodeFixTest leading to Exception dotnet/roslyn-sdk#1117

Closed

mpidash reviewed Mar 2, 2024

View reviewed changes

CollinAlpert mentioned this pull request Mar 6, 2024

Make Thread.VolatileRead and Thread.VolatileWrite obsolete dotnet/runtime#99382

Merged

CollinAlpert added 2 commits March 6, 2024 22:06

Remove analyzer from test project

3be32e0

Merge branch 'main' into issue-27997

bdd26d9

# Conflicts: # src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx

Add tests for named arguments and trivia

335a3e8

Use IOperation API

4ebbd2b

Provide arguments in parameter order.

7c117bd

buyaa-n added 2 commits March 11, 2024 16:45

Revert "Provide arguments in parameter order."

bd93b35

Enable reverse order test in VB

8284db4

buyaa-n approved these changes Mar 11, 2024

View reviewed changes

buyaa-n enabled auto-merge (squash) March 11, 2024 23:49

buyaa-n merged commit d98dd32 into dotnet:main Mar 12, 2024
11 checks passed

CollinAlpert deleted the issue-27997 branch March 12, 2024 07:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add UseVolatileReadWriteAnalyzer #7043

Add UseVolatileReadWriteAnalyzer #7043

CollinAlpert commented Nov 19, 2023

stephentoub commented Nov 19, 2023

CollinAlpert commented Nov 19, 2023

mavasani commented Nov 29, 2023

codecov bot commented Dec 5, 2023 •

edited

Loading

CollinAlpert commented Feb 7, 2024

buyaa-n commented Mar 2, 2024 •

edited

Loading

buyaa-n commented Mar 2, 2024 •

edited

Loading

CollinAlpert commented Mar 2, 2024

mpidash commented Mar 2, 2024

mpidash Mar 2, 2024

mpidash Mar 2, 2024

mpidash Mar 2, 2024

mpidash Mar 2, 2024

mpidash Mar 2, 2024

mpidash Mar 2, 2024

mpidash Mar 2, 2024

buyaa-n commented Mar 4, 2024

buyaa-n commented Mar 4, 2024

CollinAlpert commented Mar 6, 2024

CollinAlpert commented Mar 6, 2024

buyaa-n commented Mar 6, 2024

mavasani commented Mar 7, 2024

CollinAlpert commented Mar 7, 2024

buyaa-n commented Mar 7, 2024 •

edited

Loading

CollinAlpert commented Mar 7, 2024 •

edited

Loading

buyaa-n commented Mar 7, 2024

CollinAlpert commented Mar 11, 2024

buyaa-n commented Mar 11, 2024

buyaa-n left a comment

	MicrosoftNetCoreAnalyzersResources.UseVolatileReadTitle
	nameof(MicrosoftNetCoreAnalyzersResources.UseVolatileReadTitle)

	protected override bool TryGetThreadVolatileReadWriteMemberAccess(SyntaxNode invocation, string methodName, [NotNullWhen(true)] out SyntaxNode? memberAccess)
	protected sealed override bool TryGetThreadVolatileReadWriteMemberAccess(SyntaxNode invocation, string methodName, [NotNullWhen(true)] out SyntaxNode? memberAccess)

	public override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer;
	public sealed override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer;


		protected abstract bool TryGetThreadVolatileReadWriteMemberAccess(SyntaxNode invocation, string methodName, [NotNullWhen(true)] out SyntaxNode? memberAccess);

		public override ImmutableArray<string> FixableDiagnosticIds { get; } = ImmutableArray.Create("SYSLIB0054");

	public override ImmutableArray<string> FixableDiagnosticIds { get; } = ImmutableArray.Create("SYSLIB0054");
	public sealed override ImmutableArray<string> FixableDiagnosticIds { get; } = ImmutableArray.Create("SYSLIB0054");

Add UseVolatileReadWriteAnalyzer #7043

Add UseVolatileReadWriteAnalyzer #7043

Conversation

CollinAlpert commented Nov 19, 2023

stephentoub commented Nov 19, 2023

CollinAlpert commented Nov 19, 2023

mavasani commented Nov 29, 2023

codecov bot commented Dec 5, 2023 • edited Loading

Codecov Report

CollinAlpert commented Feb 7, 2024

buyaa-n commented Mar 2, 2024 • edited Loading

buyaa-n commented Mar 2, 2024 • edited Loading

CollinAlpert commented Mar 2, 2024

mpidash commented Mar 2, 2024

mpidash Mar 2, 2024

Choose a reason for hiding this comment

mpidash Mar 2, 2024

Choose a reason for hiding this comment

mpidash Mar 2, 2024

Choose a reason for hiding this comment

mpidash Mar 2, 2024

Choose a reason for hiding this comment

mpidash Mar 2, 2024

Choose a reason for hiding this comment

mpidash Mar 2, 2024

Choose a reason for hiding this comment

mpidash Mar 2, 2024

Choose a reason for hiding this comment

buyaa-n commented Mar 4, 2024

buyaa-n commented Mar 4, 2024

CollinAlpert commented Mar 6, 2024

CollinAlpert commented Mar 6, 2024

buyaa-n commented Mar 6, 2024

mavasani commented Mar 7, 2024

CollinAlpert commented Mar 7, 2024

buyaa-n commented Mar 7, 2024 • edited Loading

CollinAlpert commented Mar 7, 2024 • edited Loading

buyaa-n commented Mar 7, 2024

CollinAlpert commented Mar 11, 2024

buyaa-n commented Mar 11, 2024

buyaa-n left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 5, 2023 •

edited

Loading

buyaa-n commented Mar 2, 2024 •

edited

Loading

buyaa-n commented Mar 2, 2024 •

edited

Loading

buyaa-n commented Mar 7, 2024 •

edited

Loading

CollinAlpert commented Mar 7, 2024 •

edited

Loading